Test for RepeatDataset #26415
Test for RepeatDataset #26415
Conversation
| NodeDef node_def_; | ||
| }; | ||
|
|
||
| struct TestParam { |
| }; | ||
|
|
||
| // Test case 1: finite repetition. | ||
| TestParam TestCase1() { |
There was a problem hiding this comment.
s/TestCase1/FiniteRepeatTestCase/ ... and similarly for the other test cases
| return Status::OK(); | ||
| } | ||
|
|
||
| Status CreateDatasetFromTestCase(const TestParam &test_case) { |
| return Status::OK(); | ||
| } | ||
|
|
||
| Status CreateIteratorFromTestCase(const TestParam &test_case) { |
| return Status::OK(); | ||
| } | ||
|
|
||
| std::unique_ptr<OpKernel> dataset_kernel_; |
There was a problem hiding this comment.
Please avoid storing this state in member variables. There is no need to do this and it hurts readability.
| std::vector<Tensor> out_tensors; | ||
|
|
||
| if (test_case.count < 0) { | ||
| int fake_infinite_repetition = 100; |
There was a problem hiding this comment.
instead of a cryptic variable name, add a comment here that says that we test only test a finite number of steps of the infinite sequence, and then use a for loop.
- Change the class name and function name to improve readability - Remove dataset_kernel_ member variable - Refactor the forever repeat test
|
@jsimsa Thanks for your review! The comments are addressed in this commit. Could you have a look when you have time? |
| TF_RETURN_IF_ERROR( | ||
| CreateIteratorContext(dataset_kernel_ctx_.get(), &iterator_ctx_)); | ||
| TF_RETURN_IF_ERROR( | ||
| dataset_->MakeIterator(iterator_ctx_.get(), "Iterator", &iterator_)); | ||
| return Status::OK(); | ||
| } | ||
|
|
||
| std::unique_ptr<OpKernel> dataset_kernel_; | ||
| std::unique_ptr<OpKernelContext> dataset_kernel_ctx_; |
There was a problem hiding this comment.
My comment was meant for all of the internal state (dataset_kernel_, dataset_kernel_ctx_, and iterator_ctx_).
While writing the code this way makes it less verbose, it makes it harder to understand what is going on in case someone would like to modify the code.
|
@jsimsa This commit removes the internal member variables, and move the code in the helper class to each test to improve the code readability. Could you have a look at the changes? Any suggestions are very welcome. |
rthadur
added
kokoro:force-run
PiperOrigin-RevId: 237914360
This PR adds the test for the kernel of
RepeatDataset.cc @rachellim @jsimsa